-
Notifications
You must be signed in to change notification settings - Fork 66
fix(autocomplete): gracefully handle missing values when parsing highlights #1418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
WalkthroughAdds runtime guards and new union types to skip undefined/non-primitive Meilisearch _highlightResult entries, refactors highlight mapping to mutate an accumulator only when highlight Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant fetchFn as fetchMeilisearchResults
participant Guard as isDefinedHighlightValue
participant Mapper as mapOneOrMany
participant Meta as calculateHighlightMetadata
Client->>fetchFn: request search results
fetchFn->>fetchFn: receive raw _highlightResult
loop per attribute
fetchFn->>Guard: check attribute highlight entry
alt defined value
Guard-->>fetchFn: true
fetchFn->>Mapper: map one or many values
Mapper-->>Meta: for each value compute metadata
Meta-->>fetchFn: metadata
fetchFn->>fetchFn: assign acc[field] = metadata
else undefined / non-primitive
Guard-->>fetchFn: false
fetchFn->>fetchFn: skip attribute (no assignment)
end
end
fetchFn-->>Client: return results with sanitized highlights
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)packages/autocomplete-client/src/search/fetchMeilisearchResults.ts (1)
packages/autocomplete-client/src/search/__tests__/fetchMeilisearchResults.test.ts (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/autocomplete-client/src/search/fetchMeilisearchResults.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/autocomplete-client/src/search/fetchMeilisearchResults.ts (1)
packages/autocomplete-client/src/constants/index.ts (2)
HIGHLIGHT_PRE_TAG(1-1)HIGHLIGHT_POST_TAG(2-2)
🔇 Additional comments (3)
packages/autocomplete-client/src/search/fetchMeilisearchResults.ts (3)
153-164: LGTM: Type definitions correctly model the problem.The union type structure cleanly handles both scalar and array highlight results that may or may not have a
valueproperty. The comment usefully documents the server behavior this is working around.
67-88: LGTM: Defensive handling and clean refactoring.The early return on lines 70-72 gracefully skips highlight results without defined values, preventing type errors. The refactored logic using
mapOneOrManyand direct mutation of the accumulator is both clearer and more efficient than creating intermediate objects.Line 82's access to
highlightResult.valueis safe becauseisDefinedHighlightValuehas confirmed its presence.
148-151: LGTM: Clean, reusable helper.The
mapOneOrManyabstraction correctly handles both scalar and array values with appropriate type inference.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @jezzzm and thank you for your contribution 🙌
Pull Request
Related issue
Fixes #1417
What does this PR do?
valuekey in some attributes for highlightingvalueas a keyPR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit